-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch to clingo pypi package #123
Conversation
…depending on manual installation of clingo(gringo) binary
Hi @anubhav-cs, thanks for this. I am merging into Miquel. |
What was the motivation behind this? I'm worried that we may face issues wrapping this up for in-browser use... |
What says on the tin
|
Aye, but just out of convenience? It the same clingo binary that ships via pip? If so, why not just locate it differently and have a two-line change to nab the path right and include in the requirements? I think I'm missing some broader consequence here. Any new python package requires rebuilding that dependency (and following the chain of what it requires), so potentially a major setback for browser embeds. |
Unless some third party is impersonating the
Because it is not the (de facto) standard way of handling packages in
If I were you I would consider the possibility that there is no such consequence.
IIRC the "tarski in the browser" use case 1) Windows-only and 2) depending heavily on Anaconda? Is that correct? In any case, the |
Nah, not the use-case and it (gringo) is actually something we use (for grounding). Concretely, being able to get it all going in the browser is what enabled this award-winning plugin, and opens the door to smart grounding client-side in the browser for anyone using the online editor. |
So little drama that here is the change 1d4ab53 If there's a subset of @anubhav-cs could you please check that the |
Still doesn't help...the option is required in this case :P |
Then there you have your new default way of installing |
The usage has fundamentally changed (e.g., here), no? Are you suggesting that a non-gringo tarski with gringo binary added manually should "just work" after this merge? |
@miquelramirez Yes it works. |
Well, that's an entirely different matter @haz. @anubhav-cs you will need to add as a fallback the old |
@miquelramirez Easy peasy, will create a pull request soon. |
Merci both! We could have potentially just frozen the version we include, but that's no fun...means all the latest/greatest stays out of the browser and editor. |
I have created the PR #124 with the fallback method. |
def __init__(self, name): | ||
self.app_name = name | ||
|
||
def main(self, ctl, files): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Innocent question: do we need all this code if we're just simply using the default behavior of clingo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same question. Going by the documentation this seems to be the recommended way to get the default clingo app behavior https://potassco.org/clingo/python-api/current/clingo/application.html
Also, I checked it. The pip package doesn't include the gringo binary, instead it has a platform specific library with a python wrapper. So, we cannot nab the path to the binary.
""" | ||
class WrapperClingo(Application): | ||
def __init__(self, name): | ||
self.app_name = name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this attribute being used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That attribute should be program_name
. That's a typo on my part. It is a member of the Application
class.
Alters the clingo_wrapper, removing the requirement to manually install clingo(gringo) binary.